Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

annotate dispatch method entry point with @Generated #3080

Merged
merged 4 commits into from
Aug 4, 2024
Merged

Conversation

LorenzoBettini
Copy link
Contributor

Closes #3077

I put the new Generated annotation in xbase.lib instead of xtend.lib because it could be useful in general for Xbase languages

@LorenzoBettini LorenzoBettini requested a review from szarnekow June 7, 2024 09:57
@szarnekow
Copy link
Contributor

I don't really like the generic name @Generated - instead I'd prefer a more targeted that could even carry additional information, e.g. @GeneratedDispatchMethod(dispatchCase="_myMethod", arity=3) that could be used to unambiguously detect dispatch cases from generated Java code. What do you think about that?
Can Sonar consider meta-annotations, e.g. @MyAnnotation if MyAnnotation is annotated with @XtextGenerated or similar?

@LorenzoBettini
Copy link
Contributor Author

I don't really like the generic name @Generated - instead I'd prefer a more targeted that could even carry additional information, e.g. @GeneratedDispatchMethod(dispatchCase="_myMethod", arity=3) that could be used to unambiguously detect dispatch cases from generated Java code. What do you think about that?

OK, but in that case the annotation should be in xtend.lib, right? In that project all annotations are active annotations. Would it still be OK to have a standard annotation there?

Can Sonar consider meta-annotations, e.g. @MyAnnotation if MyAnnotation is annotated with @XtextGenerated or similar?

Do you mean JaCoCo or Sonar?
Do you mean to also exclude meta annotations from code coverage? In that case, what would be an example?
To be honest I'm not familiar with meta-annotations.

@cdietrich
Copy link
Contributor

how does this overlap with the preference

grafik

@szarnekow
Copy link
Contributor

OK, but in that case the annotation should be in xtend.lib

If we agree on such a specific annotation, I'd put it next to StringConcatenationClient with class retention.

meta-annotations

Here is a very concise description of meta-annotations: https://docs.spring.io/spring-framework/reference/core/beans/classpath-scanning.html#beans-meta-annotations

In a nutshell: Rather than putting the @Generated directly on the annotation target, another annotation is annotated as @Generated and that one is put on the annotation target. That would allow @Dispatcher to be used (arguably a better name than @GeneratedDispatcher).

But @cdietrich raises a fair question: Would we always emit @XbaseGenerated or whatever the name should be?

@cdietrich
Copy link
Contributor

yes. am not sure if we still have the old javax.annotation.Generated present.
am also not sure if we want the annotation to be used configurable

@LorenzoBettini
Copy link
Contributor Author

@szarnekow so a meta-annotation is just an annotation put on another annotation; if I understand correctly what you suggest, I manually tried and it didn't work:

@Retention(RetentionPolicy.CLASS)
@Target({TYPE, METHOD, CONSTRUCTOR})
public @interface Generated {
}

and

@Retention(RetentionPolicy.CLASS)
@Target({TYPE, METHOD, CONSTRUCTOR})
@Generated
public @interface Dispatcher {

	String dispatchCase();

	int arity();
}

but JaCoCo does NOT ignore the annotated method:

  @Dispatcher(dispatchCase = "_m", arity = 1)
  public String m(final Object i) {
    if (i instanceof Integer) {
      return _m((Integer)i);
    } else if (i instanceof String) {
      return _m((String)i);
    } else {
      throw new IllegalArgumentException("Unhandled parameter types: " +
        Arrays.<Object>asList(i).toString());
    }
  }

As I said in #3077 JaCoCo expects that "The name of the annotation includes Generated."

Concerning javax.annotation.Generated, related to the configuration option shown by @cdietrich above,

@Retention(SOURCE)
@Target({PACKAGE, TYPE, ANNOTATION_TYPE, METHOD, CONSTRUCTOR, FIELD, 
        LOCAL_VARIABLE, PARAMETER})
public @interface Generated {

That is a source annotation so it cannot be used to this aim.

Concerning always generating the new annotation, I'd go with always generating that. And it's unrelated to the other existing one.

The current Generated annotation, like SuppressWarning, is something handled by Xbase, while this new one would be specific of Xtend if we go with the version with GeneratedDispatcher in the name and arguments related to the dispatcher method as suggested by Sebastian. In that case, Sebastian, you say to put it next to org.eclipse.xtend2.lib.StringConcatenationClient but it's still in the project xbase.lib.

@LorenzoBettini
Copy link
Contributor Author

In the meantime, I rebased on main.
There are still a few UI tests to fix, but I'd like to know the possible directions we want to take concerning this proposal, so that I can implement them only once ;)

@LorenzoBettini
Copy link
Contributor Author

So: we'll use @XbaseGenerated and to keep the annotation with class retention policy.

@cdietrich
Copy link
Contributor

Can we check the annotation removal in lsp4j

@cdietrich
Copy link
Contributor

(Lsp4j uses xtend/xbase lib as compile time only dependencies

@LorenzoBettini
Copy link
Contributor Author

Can we check the annotation removal in lsp4j

Could you please elaborate? I don't understand what you mean

@cdietrich
Copy link
Contributor

in lsp4j the java classes generated from xtend classes
shall not make use of any classes in xbase.lib and xtend.lib

we have some code in annotation processing that takes care.
my quesiton is: do these changes break this

@LorenzoBettini
Copy link
Contributor Author

in lsp4j the java classes generated from xtend classes shall not make use of any classes in xbase.lib and xtend.lib

we have some code in annotation processing that takes care. my quesiton is: do these changes break this

Could you please point me to where we take care of this (annotation processing)?
And some pointers to lsp4j using Xtend?

@cdietrich
Copy link
Contributor

@cdietrich
Copy link
Contributor

maybe we are safe if they dont use dispatch

@szarnekow
Copy link
Contributor

We should also make this optional (enabled by default) and expose a generator config that allows to opt-out from that

@LorenzoBettini
Copy link
Contributor Author

Sorry I didn't have time yet to fix the failing tests.
I'll try to do that ASAP, but maybe tomorrow.
I'll also try to add a new option enabled by default, taking inspiration from the current options. Of course, if you have suggestions on how the option could be named...

@szarnekow
Copy link
Contributor

szarnekow commented Jun 25, 2024

Label in UI could be Annotate synthetic members with @XtextGenerated.
Option useXtextGenerated

@LorenzoBettini
Copy link
Contributor Author

@szarnekow So XtextGenerated, not XbaseGenerated?

@szarnekow
Copy link
Contributor

@szarnekow So XtextGenerated, not XbaseGenerated?

Freudian slip. XbaseGenerated

@LorenzoBettini LorenzoBettini force-pushed the lb_3077 branch 3 times, most recently from 5bcc229 to ba07d30 Compare June 25, 2024 20:03
@cdietrich
Copy link
Contributor

lsp4j does not seem to use dispatches.
and if they were the Architecture Test i have written would catch it

java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'no classes that reside in a package 'org.eclipse.lsp4j..' and not reside in a package 'org.eclipse.lsp4j.generator..' and not reside in a package 'org.eclipse.lsp4j.test..' should depend on classes that reside in a package 'org.eclipse.xtext..'' was violated (1 times):
Method <org.eclipse.lsp4j.Viech.x(java.io.InputStream)> is annotated with <org.eclipse.xtext.xbase.lib.XbaseGenerated> in (Viech.java:0)

@LorenzoBettini
Copy link
Contributor Author

@cdietrich Thanks for testing it!

I'll add the compiler option soon anyway.

@LorenzoBettini
Copy link
Contributor Author

@szarnekow I've implemented the new option, the involved commit is a33af2e

Here's how the properties dialog looks like

Screenshot from 2024-06-26 13-38-12

By enabling project specific settings, and disabling the option:

Screenshot from 2024-06-26 13-40-45

By enabling the option again:

Screenshot from 2024-06-26 13-41-10

@LorenzoBettini
Copy link
Contributor Author

ping...

@LorenzoBettini
Copy link
Contributor Author

@cdietrich @szarnekow could we take a decision on this please?

@cdietrich
Copy link
Contributor

sry i have zero cappa for looking at anything atm.
the efforts for the MWE and Xtext milestones already exeed by cappa.

@LorenzoBettini LorenzoBettini added this to the Release_2.36 milestone Aug 4, 2024
@LorenzoBettini LorenzoBettini merged commit f59271c into main Aug 4, 2024
10 of 11 checks passed
@LorenzoBettini LorenzoBettini deleted the lb_3077 branch August 4, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Xtend generated code easier to cover
3 participants